[fix](fe) Fix isCountStar incorrectly treating count(null) as count(*)#62548
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
regression-test/suites/nereids_rules_p0/count_null_not_count_star/count_null_not_count_star.groovy: the new window case does not reachSimplifyWindowExpression, so it does not actually validate theisCountStar()consumer named in the PR.regression-test/suites/nereids_rules_p0/count_null_not_count_star/count_null_not_count_star.groovy: the new join case does not reachPushDownAggThroughJoin'scount(*)rewrite branch, so it also does not validate the motivating join path described in the PR.fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Count.java: this helper also gatesAggregateStrategies(COUNT_ON_INDEX/COUNT_ON_MATCH), but the new tests do not cover that implementation path yet.
Critical Checkpoints
- Goal: The one-line change in
Count.isCountStar()fixes the directcount(null)misclassification. The new unit test proves the helper behavior, but the added regression suite does not fully prove all claimed downstream consumers. - Scope: The change is small, focused, and centralized in the right helper.
- Concurrency: No concurrency, locking, or lifecycle risk is involved here.
- Config / compatibility: No new config, protocol, or storage compatibility impact.
- Parallel code paths:
isCountStar()is shared by multiple rewrite and implementation rules. The code updates them centrally, but coverage is still incomplete for the window, join, and count-on-index consumers. - Special conditions: The new null-literal exclusion is the right semantic distinction.
- Tests:
CountLiteralRewriteTestimproves direct coverage, and the union regression is useful. However the new window and join regressions do not hit the named rewrite rules, and there is still no coverage for theCOUNT_ON_INDEXfast path. I also could not run FE UT successfully in this runner becausethirdparty/installed/bin/protocis missing. - Result files: The new
.outfile is consistent with the SQL in the added suite. - Observability / transaction / persistence / FE-BE variable passing: Not applicable for this change.
- Performance: No new performance concern found.
- Other: I did not find another blocking correctness bug in the code change itself; the blocking issue is coverage quality for the affected consumers.
Requesting changes so the added tests exercise the actual rewrite / implementation paths that motivated this fix.
| ) t | ||
| """ | ||
|
|
||
| // count(null) as window function |
There was a problem hiding this comment.
This window query does not actually exercise SimplifyWindowExpression. That rule only rewrites when the partition by keys are proven unique (window.getLogicalProperties().getTrait().isUnique(partitionSlotSet)), but this test has no partition keys, so it would skip the rule before and after the fix. Please add a unique-partition case (or a direct FE rule test) if the goal is to cover the isCountStar() consumer in SimplifyWindowExpression.
There was a problem hiding this comment.
Fixed. Window test kept as a correctness check (count(null)=0 in window context). SimplifyWindowExpression requires provably unique partition keys which is hard to set up in regression tests — the core isCountStar() behavior is already covered by FE unit test testCountNullIsNotCountStar.
| select pk, count(null) over (order by pk) from count_null_test order by pk | ||
| """ | ||
|
|
||
| // count(null) through join must still return 0, not get rewritten by PushDownAggThroughJoin |
There was a problem hiding this comment.
This join case does not reach PushDownAggThroughJoin's count(*) rewrite branch. With empty GROUP BY, PushDownAggThroughJoin#pushAgg returns null as soon as countStars is non-empty, so even the old buggy count(null) -> isCountStar() classification would not rewrite this query. To cover the motivating join scenario, the test needs a grouped join (or a plan test in PushDownCountThroughJoinTest) that actually enters the count(*) rewrite path.
There was a problem hiding this comment.
Fixed. Replaced with a grouped join: select t1.pk, count(null), count(*) from ... group by t1.pk. Also added FE plan test testCountNullNotPushedDown in PushDownCountThroughJoinTest.java applying only PushDownAggThroughJoin to verify count(null) is NOT pushed down.
### What problem does this PR solve? Issue Number: close #xxx Problem Summary: Count.isCountStar() returns true for count(null) because it only checks `child(0) instanceof Literal` without excluding NullLiteral. Since count(null) always returns 0 (it counts non-null values of a constant null), it is semantically different from count(*) which counts all rows. This could cause incorrect results if downstream rewrite rules (e.g. PushDownAggThroughJoin, SimplifyWindowExpression, PushCountIntoUnionAll) use isCountStar() before CountLiteralRewrite has a chance to rewrite count(null) to 0. ### Release note Fixed count(null) being incorrectly treated as count(*) in query optimizer, which could lead to wrong results in certain rewrite scenarios. ### Check List (For Author) - Test: Unit Test / Regression test - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
yujun777
force-pushed
the
fix/count-null-is-not-count-star
branch
from
ca09b74 to
aed0d9c
Compare
|
run buildall |
|
/review |
github-actions
Bot
removed
the
approved
There was a problem hiding this comment.
I agree with the semantic fix in Count.isCountStar() itself: excluding null literals is the right change, and the implementation is small and focused. The blocking issue is that the new coverage does not actually prove all of the downstream optimizer paths called out in the PR description.
count_null_uniondoes not exercisePushCountIntoUnionAll.CountLiteralRewriteruns earlier in the rewrite pipeline and foldscount(null)to constant0before the union-all pushdown stage sees it, so this query would still pass if that rule regressed.count_null_windowdoes not exerciseSimplifyWindowExpression. That rule only rewrites when thePARTITION BYkeys are unique; the added query has noPARTITION BY, so a regression incheckCount()/isCountStar()there would go unnoticed.- The inverted-index case only checks the negative explain. If
count(*)stopped usingCOUNT_ON_MATCH, the test would still pass becauseexplainCountStaris never asserted.
Critical Checkpoints
- Goal of current task: Prevent
count(null)from being treated ascount(*). The code change inCount.javadoes that locally, but the current tests do not fully prove the downstream paths named in the PR description. - Modification size/focus: Small and focused.
- Concurrency: Not involved in the touched code.
- Lifecycle/static initialization: Not involved.
- Config changes: None.
- Compatibility / FE-BE protocol / persistence: None.
- Parallel code paths: Yes. Several optimizer paths depend on row-count semantics. The added coverage does not actually pin
PushCountIntoUnionAll, does not hitSimplifyWindowExpression, and only partially checks theCOUNT_ON_INDEXpath. - Special conditions: The new null-literal guard is semantically appropriate.
- Test coverage: Incomplete for the risky paths above.
- Test result updates: The added
.outfile matches the SQL results that are asserted. - Observability: No change needed.
- Transactionality / data writes / new FE-BE variables: Not applicable.
- Performance: No concern from the code change itself.
- Other issues: None beyond the missing coverage.
Please tighten the new tests so they actually exercise the optimizer branches this PR is claiming to fix.
| order_qt_count_mixed """select count(null), count(*), count(a), count(b) from count_null_test""" | ||
|
|
||
| // count(null) in subquery with union all | ||
| order_qt_count_null_union """ |
There was a problem hiding this comment.
count(null) has already been rewritten to constant 0 by CountLiteralRewrite before the eager-aggregation stage reaches PushCountIntoUnionAll, so this query does not actually pin the union-all pushdown path mentioned in the PR description. It would still pass even if PushCountIntoUnionAll mishandled count(null). Please add a rule-level FE test or a plan assertion that exercises that rewrite directly.
There was a problem hiding this comment.
Fixed. Added FE unit test testCountNullPushedAsCountNullNotCountStar in PushCountIntoUnionAllTest.java.
Note: CountLiteralRewrite does not rewrite count(null) to 0 in the no-GROUP-BY case. At line 66 of CountLiteralRewrite.java, it guards with aggFuncs.isEmpty() — when count(null) is the only aggregate and there are no GROUP BY keys, the resulting aggregate would have zero output expressions, so the rule returns null (no-op). PushCountIntoUnionAll therefore does see count(null) in this case.
The new FE unit test directly verifies: (1) PushCountIntoUnionAll fires (Sum0 appears in upper agg), and (2) child agg expressions do NOT contain a Count with isCountStar=true.
| """ | ||
|
|
||
| // count(null) as window function should still return 0 | ||
| order_qt_count_null_window """ |
There was a problem hiding this comment.
This window case does not reach SimplifyWindowExpression. That rule only rewrites when the PARTITION BY keys are unique, and an empty partition-key set is not treated as unique. With count(null) over (order by pk) the regression only checks result correctness, not the optimizer branch called out in the PR description. Please add a PARTITION BY case on a unique key such as pk so checkCount() / isCountStar() is exercised.
There was a problem hiding this comment.
Fixed. Added a UNIQUE KEY table count_null_uniq_test with PARTITION BY pk so isUnique({pk}) returns true and SimplifyWindowExpression does fire. The test asserts count(null) over (partition by pk) = 0 for each row with explicit assertEquals(0L, row[1]) assertions. With the fix, isCountStar() returns false for count(null) → checkCount() returns false → window is NOT simplified → correctly returns 0.
|
|
||
| // count(*) with MATCH predicate may use COUNT_ON_MATCH optimization | ||
| // count(null) with same predicate must NOT — verify via explain | ||
| def explainCountStar = sql """ |
There was a problem hiding this comment.
Only the negative explain is asserted here. If the positive optimization for count(*) stopped producing COUNT_ON_MATCH, this test would still pass because explainCountStar is unused. Please also assert that the count(*) explain contains COUNT_ON_MATCH or COUNT_ON_INDEX so this case proves the optimization path is actually exercised.
There was a problem hiding this comment.
Fixed. Replaced the explain shape plan + assertFalse(contains 'COUNT_ON_MATCH') block — which was trivially always-true since explain shape plan calls shapeInfo() (returns class name only, never includes pushAggOp) — with proper explain {} DSL blocks:
count(*)assertscontains "pushAggOp=COUNT_ON_INDEX"(positive)count(null)assertsnotContains "pushAggOp=COUNT_ON_INDEX"(negative)
Note: PhysicalPlanTranslator maps COUNT_ON_MATCH → TPushAggOp.COUNT_ON_INDEX, so explain output shows pushAggOp=COUNT_ON_INDEX for both COUNT_ON_INDEX and COUNT_ON_MATCH physical paths. The positive assertion proves the optimization is actually exercised for count(*).
|
run feut |
|
run vault_p0 |
FE Regression Coverage ReportIncrement line coverage |
… optimizer paths ### What problem does this PR solve? Issue Number: close #xxx Problem Summary: Address three reviewer concerns on PR apache#62548: 1. PushCountIntoUnionAll: Add FE unit test testCountNullPushedAsCountNullNotCountStar verifying count(null) is pushed down as count(null) not as count(*). CountLiteralRewrite skips count(null) with no GROUP BY (would produce empty agg), so PushCountIntoUnionAll does see count(null). The new test explicitly pins this behavior. 2. SimplifyWindowExpression: Add regression test with PARTITION BY on a UNIQUE KEY table (count_null_uniq_test). The rule only fires when partition keys are proven unique; this test directly exercises checkCount() → isCountStar() for count(null). 3. COUNT_ON_INDEX: Replace explain shape plan (which only shows class name, not pushAggOp) with the explain{} DSL block. Now asserts count(*) DOES use pushAggOp=COUNT_ON_INDEX and count(null) does NOT, making the test non-trivial. ### Release note None ### Check List (For Author) - Test: Unit Test / Regression test - Behavior changed: No - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
run buildall |
|
/review |
There was a problem hiding this comment.
No blocking issues found.
Critical Checkpoints
- Goal: The change prevents
count(null)from being treated ascount(*)in sharedCount.isCountStar()logic. The one-line fix is correct, and the added UT/regression coverage now exercises the direct helper behavior plus the union-all, join, window, and count-on-index consumers called out in the PR. - Scope: The modification is small, focused, and centralized in the right helper.
- Concurrency: No concurrency, locking, or lifecycle risk is involved in the touched code.
- Lifecycle / static init: No special lifecycle or static initialization impact.
- Config: No new configuration or dynamic-config behavior.
- Compatibility: No FE-BE protocol, persistence, or rolling-upgrade compatibility impact.
- Parallel code paths: I checked the current
isCountStar()consumers; the shared helper fix covers the relevant rewrite and implementation paths, and I did not find another reachable correctness gap forcount(null). - Special conditions: Excluding null literals is the right semantic distinction.
count(*)and non-null literal counts keep their existing behavior. - Test coverage: The new FE tests and regression suite are targeted and sufficient for this fix. The new
.outfile is consistent with the deterministic queries. I could not rerun FE UT in this runner becausethirdparty/installed/bin/protocis missing, so the verification here is based on code and plan-path inspection. - Observability: No additional logs or metrics are needed for this change.
- Transactionality / data writes / FE-BE vars: Not applicable.
- Performance: No new performance concern found.
- Other: No additional blocking issues found.
|
run vault_p0 |
FE Regression Coverage ReportIncrement line coverage |
|
run nonConcurrent |
FE Regression Coverage ReportIncrement line coverage |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
#62548) ### What problem does this PR solve? Problem Summary: `Count.isCountStar()` returns `true` for `count(null)` because it only checks `child(0) instanceof Literal` without excluding `NullLiteral`. Since `count(null)` always returns 0 (it counts non-null values of a constant null), it is semantically different from `count(*)` which counts all rows. This could cause incorrect results if downstream rewrite rules (e.g. `PushDownAggThroughJoin`, `SimplifyWindowExpression`, `PushCountIntoUnionAll`) use `isCountStar()` before `CountLiteralRewrite` has a chance to rewrite `count(null)` to `0`. ### Release note Fixed `count(null)` being incorrectly treated as `count(*)` in query optimizer, which could lead to wrong results in certain rewrite scenarios. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
#62548) ### What problem does this PR solve? Problem Summary: `Count.isCountStar()` returns `true` for `count(null)` because it only checks `child(0) instanceof Literal` without excluding `NullLiteral`. Since `count(null)` always returns 0 (it counts non-null values of a constant null), it is semantically different from `count(*)` which counts all rows. This could cause incorrect results if downstream rewrite rules (e.g. `PushDownAggThroughJoin`, `SimplifyWindowExpression`, `PushCountIntoUnionAll`) use `isCountStar()` before `CountLiteralRewrite` has a chance to rewrite `count(null)` to `0`. ### Release note Fixed `count(null)` being incorrectly treated as `count(*)` in query optimizer, which could lead to wrong results in certain rewrite scenarios. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
6 participants
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
Count.isCountStar()returnstrueforcount(null)because it only checkschild(0) instanceof Literalwithout excludingNullLiteral. Sincecount(null)always returns 0 (it counts non-null values of a constant null), it is semantically different fromcount(*)which counts all rows.This could cause incorrect results if downstream rewrite rules (e.g.
PushDownAggThroughJoin,SimplifyWindowExpression,PushCountIntoUnionAll) useisCountStar()beforeCountLiteralRewritehas a chance to rewritecount(null)to0.Release note
Fixed
count(null)being incorrectly treated ascount(*)in query optimizer, which could lead to wrong results in certain rewrite scenarios.Check List (For Author)
CountLiteralRewriteTest(3 tests pass, including newtestCountNullIsNotCountStar)count_null_not_count_starcovering basic, group by, mixed, union all, window, and join paths